Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: new BlockWriter implementation for block-as-file #355

Draft
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

ata-nas
Copy link
Contributor

@ata-nas ata-nas commented Nov 19, 2024

Description:

This PR aims to introduce the new BlockAsFileWriter with the initial goal to only be able to write blocks as file. In subsequent PRs this functionality will be improved and cleaned up. You can see #309 for more details on the full functionality that will be implemented.

Related issue(s):

Fixes #281 #319 #348

Notes for reviewer:

TBD

Checklist


  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@ata-nas ata-nas added the Block Node Issues/PR related to the Block Node. label Nov 19, 2024
@ata-nas ata-nas added this to the 0.3.0 milestone Nov 19, 2024
@ata-nas ata-nas self-assigned this Nov 19, 2024
@ata-nas ata-nas linked an issue Nov 19, 2024 that may be closed by this pull request
@ata-nas ata-nas force-pushed the 281-new-blockwriter-implementation-for-block-as-file branch from 1e81bdb to 2d82213 Compare November 19, 2024 12:45
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
@ata-nas
Copy link
Contributor Author

ata-nas commented Nov 19, 2024

Intermediate thoughts at 9fa5c10:

  • currently I am interested only into getting the new writer to write blocks as files (not yet implemented)
  • the new writer currently implements BlockWriter<List<BlockItem>> in order for me to get it to work as fast as possible, subject to change in the future with BlockWriter<Block>
  • introduced a PathResolver, resolving the path to a block, be it a block-as-file or a block-as-dir is a core function of the readers/writers, I extracted that logic in a new component so that it can be thoroughly tested on it's own and also to provide a lot of flexibility by allowing us to mock this functionality when we test the readers/writers itself
  • the PathResolver also separates the concern of resolving block paths from the reader/writer (single responsibility is improved)
  • the object creation (type of) of the readers, writers, removers and path resolvers is done now via a configurable value
  • added abstract classes, there will be reusable logic (in some places already exists) and will help us to formalize what dependencies the given service will need, regardless of specific implementation

@ata-nas
Copy link
Contributor Author

ata-nas commented Nov 20, 2024

Update at 5c0a707:

  • we now have simple logic to write the block as file to the filesystem
  • in this v1 commit, I aim to simply start writing to the fs
  • what am I missing
  • what additional handles I need to make
  • what does the cleanup at this stage looks like
  • what can be removed
  • what about cases where we receive a list of block items, but the block header and/or proof is missing, technically this means that I must have received the header previously and expect the proof later, the block stream is a continuous stream of block items delimited by block header and block proof

Copy link
Member

@jsync-swirlds jsync-swirlds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few areas that could be improved.

server/docs/configuration.md Outdated Show resolved Hide resolved
Comment on lines 104 to 107
} catch (final Exception e) {
// todo handle properly
throw new RuntimeException(e);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you have a very good reason, do not catch Exception, ever (a vast array of bugs in real systems come from this seemingly innocuous block).
Instead, handle any specific exceptions you can, declare any checked exceptions necessary, and let callers handle things from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The try-catch here would probably be greatly reworked or removed from here. This was done just to help develop the core functionality.

* @param blockPathResolver valid, {@code non-null} instance of {@link BlockPathResolver}
* @return a new instance of {@link BlockAsFileWriterBuilder}
*/
public static BlockAsFileWriterBuilder newBuilder(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder constructors should not, in general, take parameters. The whole point of a "builder" is to start empty and set each value as needed (chained by returning the builder from each set method) before a build produces the result value.

What you've written here is an oddly structured factory, rather than a builder.

Copy link
Contributor Author

@ata-nas ata-nas Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree about the builders, in fact, I prefer to implement Joshua Bloch's builder where we have Builder class that is internal to the class we want to build, since we can access directly the private filed of the internal builder from the target class and pass the builder itself as a constructor parameter.

I also agree that the builder should start "empty" and we set each value as needed.

I do not agree however that the builder constructor or static method that gets us the builder instance should have no arguments. There are some things that are mandatory for the given instance that will be constructed. Without forcing the caller to supply them, it is very error prone. The optional stuff should be settable later.

In our case it really looks like an odd factory, in fact I was thinking if we need the builders at all, we could just have static of() or from() or similar method directly inside the target class and omit the builders. The only place where we currently have an optional value is the folderPermissions which I will remove the possibility to be set externally, they shall remain an internal property of the class. All other builders are in the same fashion.

There are two views here in my opinion.

  1. We simplify and remove the builders as they are not needed at all at this point, no need to support them.
  2. We keep the builders as they are with the thinking that if in the future will appear optional dependencies they will be introduced within the builder as setters and our refactoring will be minimal.

UNRELATED SIDE NOTE: In some rare cases where we have a very complex building logic that requires some things to be set before others or when setting something then we must proceed with additional settings for what we just set, then I like to do interactive builders where I have multiple interfaces with specific setters and my builder implements all giving me the possibility to guide the caller through the possible options at any given step, returning a specific interface. (not applicable at all to us, just an interesting side note)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the well reasoned reply. Here are my thoughts.

  1. Removing the builders and using a static of method makes sense in these small-value cases where you want all the required values set. My comment was that a constructor, specifically, should not have parameters; a static method is a secondary pattern (builder-factory-method), and it is fine to have parameters (in fact practically required) there.
    • It also makes sense to have a builder that is properly designed, internal to the class to build, and accessible only as a return value from an of method that takes the required parameters; this also avoids the perceived need to have a constructor with parameters while retaining regular "builder" semantics.
  2. Re: the side note, I am not a huge fan of interactive builders with multiple interfaces (I've seen that become a nest of bugs that breeds more too many times). If there is an ordering or containment requirement, the intent of a builder would, in my opinion, be to remove the ordering/containment requirement and allow values to be set in any order, but then apply them to the created object in the correct order and containment during the build method. A build method might, in this setup, construct an entire tree of objects in order to meet all the requirements for a particular highly complex object build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsync-swirlds awesome! Thanks also for your insights! I like the combination of the of method that will get us an internal builder. I will implement that, it will clean up a lot of things! Just so that there is no confusion on my side:

  1. Do you propose that I will now remove the external builders and have a static of method that will be part of the target class, from where we will supply all mandatory dependencies AND since there are currently no cases (or will not be once I remove the folderPermissions) of optional dependencies, the of method alone is sufficient. Once we have an optional dependency for the given class, the of method will then return a newly made internal to the target class "Joshua Bloch" style builder and it will be an easy refactor.

OR

  1. Same as above, I remove the external builders, have an of method in the target class, but proactively create the internal builders and simply call the build method where I want an instance of the target class, despite the fact that currently we have no optional deps.

?

In any case, I am generally pro for static instance creation methods.

Let me know, thanks again, insightful thread! :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend starting with option (1), and we can always adjust later if needed.

Copy link
Contributor Author

@ata-nas ata-nas Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Done. Quite the refactoring it was, took a while, but everything is a lot cleaner :). One note, for the places where are no input params, I have used the method name 'newInstnace', the name 'of' and then passing nothing really did not fit at all. Since now everything is wired, a simple rename of all usages of 'newInstance' is trivial and takes milliseconds.

server/src/main/resources/app.properties Outdated Show resolved Hide resolved
@ata-nas
Copy link
Contributor Author

ata-nas commented Nov 26, 2024

@jsync-swirlds thanks for the review, I have missed a couple of places where I could clean up additionally, however at some places it looks like you were looking at older changes, files were amended before you have made your review. I have also included #319 here and will bring additional changes to the exception handling to clean that up as well.

Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 71.57895% with 54 lines in your changes missing coverage. Please review.

Project coverage is 94.02%. Comparing base (c0ee61f) to head (efb95eb).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...sistence/storage/write/BlockAsLocalFileWriter.java 0.00% 34 Missing ⚠️
...server/persistence/PersistenceInjectionModule.java 30.76% 16 Missing and 2 partials ⚠️
...istence/storage/remove/BlockAsLocalDirRemover.java 80.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #355      +/-   ##
============================================
- Coverage     97.77%   94.02%   -3.75%     
- Complexity      343      358      +15     
============================================
  Files            70       76       +6     
  Lines          1256     1340      +84     
  Branches         88       91       +3     
============================================
+ Hits           1228     1260      +32     
- Misses           19       69      +50     
- Partials          9       11       +2     
Files with missing lines Coverage Δ
...a/com/hedera/block/common/utils/Preconditions.java 100.00% <100.00%> (ø)
...er/config/ServerMappedConfigSourceInitializer.java 100.00% <ø> (ø)
...edera/block/server/metrics/MetricsServiceImpl.java 100.00% <100.00%> (ø)
...rver/persistence/StreamPersistenceHandlerImpl.java 100.00% <100.00%> (ø)
.../persistence/storage/PersistenceStorageConfig.java 100.00% <100.00%> (ø)
...ence/storage/path/BlockAsLocalDirPathResolver.java 100.00% <100.00%> (ø)
...nce/storage/path/BlockAsLocalFilePathResolver.java 100.00% <100.00%> (ø)
...ersistence/storage/path/NoOpBlockPathResolver.java 100.00% <100.00%> (ø)
...ersistence/storage/read/BlockAsLocalDirReader.java 96.42% <100.00%> (ø)
...rsistence/storage/read/BlockAsLocalFileReader.java 100.00% <100.00%> (ø)
... and 7 more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Block Node Issues/PR related to the Block Node.
Projects
None yet
2 participants